-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Components: add useUpdateEffect
to exhaustive-deps
lint check
#45771
base: trunk
Are you sure you want to change the base?
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
Size Change: +20 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments that could potentially simplify the changes, although it'd be great if @mirka could also have a look at this PR and smoke-test it, given her familiarity with the component in the past weeks
|
||
// Sync incoming value with groupContext.state. | ||
useUpdateEffect( () => { | ||
if ( value !== groupContext.state ) { | ||
groupContext.setState( value ); | ||
} | ||
}, [ value ] ); | ||
}, [ value, groupContext ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since groupContext.setState
is effectively setSelectedValue
, I wonder if we could just swap the call inside the hook to use setSelectedValue
instead of groupContext.setState
. That way, ESLint should not complain about a missing dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any problem with that approach! One thing to bear in mind though is that to actually eliminate the the groupContext
dependency, we'd also need to replace groupContext.state
with selectedValue
. At that point, selectedValue
becomes a dependency, so we've kind of come full circle and are still adding a second dependency.
That said, if we feel that selectedValue
and setSelectedValue
are more readable than the current groupContext
values, I'm happy to make that change! 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was thinking that using selectedValue
and setSelectedValue
fits my mental model better:
value
orselectedValue
change- the hook calls
setSelectedValue
- this causes a re-render, which causes the
useMemo
hook to re-run and updategroupContext
with the latestselectedValue
groupContext
is not really used internally by the component, but only created in order to pass it to the context provider, for child components to use. This allows also to move all context props underuseMemo
, in order to avoid creating a new context object every render
Something like this
diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
index d4a040580e..3708f9b7bb 100644
--- a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
+++ b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
@@ -22,7 +22,10 @@ import ToggleGroupControlBackdrop from './toggle-group-control-backdrop';
import ToggleGroupControlContext from '../context';
import { useUpdateEffect } from '../../utils/hooks';
import type { WordPressComponentProps } from '../../ui/context';
-import type { ToggleGroupControlMainControlProps } from '../types';
+import type {
+ ToggleGroupControlMainControlProps,
+ ToggleGroupControlContextProps,
+} from '../types';
function UnforwardedToggleGroupControlAsButtonGroup(
{
@@ -47,41 +50,39 @@ function UnforwardedToggleGroupControlAsButtonGroup(
'toggle-group-control-as-button-group'
).toString();
const [ selectedValue, setSelectedValue ] = useState( value );
- const groupContext = useMemo(
- () => ( {
- baseId,
- state: selectedValue,
- setState: setSelectedValue,
- } ),
- [ baseId, selectedValue ]
- );
const previousValue = usePrevious( value );
- // Propagate groupContext.state change.
+ // Propagate selectedValue changes.
useUpdateEffect( () => {
- // Avoid calling onChange if groupContext state changed
+ // Avoid calling onChange if the selectedValue changed
// from incoming value.
- if ( previousValue !== groupContext.state ) {
- onChange( groupContext.state );
+ if ( previousValue !== selectedValue ) {
+ onChange( selectedValue );
}
- }, [ groupContext.state, previousValue, onChange ] );
+ }, [ selectedValue, previousValue, onChange ] );
- // Sync incoming value with groupContext.state.
+ // Sync incoming value with selectedValue.
useUpdateEffect( () => {
- if ( value !== groupContext.state ) {
- groupContext.setState( value );
+ if ( value !== selectedValue ) {
+ setSelectedValue( value );
}
- }, [ value, groupContext ] );
+ }, [ value, selectedValue ] );
+
+ // Expose selectedValue getter/setter via context
+ const groupContext: ToggleGroupControlContextProps = useMemo(
+ () => ( {
+ baseId,
+ state: selectedValue,
+ setState: setSelectedValue,
+ isBlock: ! isAdaptiveWidth,
+ isDeselectable: true,
+ size,
+ } ),
+ [ baseId, selectedValue, isAdaptiveWidth, size ]
+ );
return (
- <ToggleGroupControlContext.Provider
- value={ {
- ...groupContext,
- isBlock: ! isAdaptiveWidth,
- isDeselectable: true,
- size,
- } }
- >
+ <ToggleGroupControlContext.Provider value={ groupContext }>
<View
aria-label={ label }
{ ...otherProps }
How does the above proposed change look to y'all? (cc @mirka )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, it looks like at least one of our failing tests on this PR is being caused by adding selectedValue
(or groupContext
under the previous implementation) to the dependency array of the second useUpdateEffect
.
It doesn't look like the deselection is actually working. The test clicks the button once, then clicks it again, but the pressed
state doesn't look like it's updating. I'll keep investigating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marco's suggestions make perfect sense to me 👍
Looks like the test failure is because the test is doing an uncontrolled usage. Uncontrolled usages would legitimately break if we added selectedValue
to the deps array.
The second useEffect is intended to sync incoming value
prop changes to the internal component state. (The first useEffect is the inverse — it syncs internal state up to the outer controlled value
via onChange
.)
We only want the effect to fire when value
changes, not when selectedValue
changes. So something like this is probably what we want instead:
// Sync incoming value with selectedValue.
useUpdateEffect( () => {
if ( previousValue !== value ) {
setSelectedValue( value );
}
}, [ value, previousValue ] );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. yes that makes sense. Thank you!
Changes applied, waiting on CI. 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested these changes on Storybook — it seems like the onChange
callback gets called twice every time the value changes
I wonder if using useControlledValue
instead of the custom logic would help here with syncing controlled and uncontrolled updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, promising idea. I've pushed an attempt at this change in 3acacad. I had to address a type discrepancy on the setter function, let me know how it looks!
edit: updated commit sha after rebasing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, update on the useControlledValue
approach. It proved to be problematic. Expected behavior is that you click a button once, it gets selected. Click it again, it deselects.
With useControlledValue
, the second click doesn't do anything (at least not from a UI perspective). It's the third click that actually deactivates the button and resets the value.
I believe this is happening because useControlledValue
looks for the value prop to be undefined
, and ToggleGroupControlOptionBase
intentionally passes undefined
when deselecting. This means useControlledValue
sees undefined
as the value being passed in, assumes there is no value, and treats it as an uncontrolled component.
We can work around that by having ToggleGroupControlOptionBase
pass an empty string instead of undefined
, and make one small test fix. Before I push that change though, I wondered what your thoughts on that might be @ciampo @mirka
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not to have to pass an empty string instead of undefined
, since that is rather arbitrary and not a solution that can be adopted in the general case (e.g., what if our state was a number
type?). In fact, it's probably worth fixing this limitation in useControlledValue
itself (not in this PR).
In our specific case, I feel like the most straightforward fix for the double onChange problem is something like this:
// Propagate selectedValue change.
useUpdateEffect( () => {
// Only call onChange when uncontrolled
if ( previousValue === value ) {
onChange( selectedValue );
}
}, [ selectedValue, previousValue, value, onChange ] );
This seems to be working in my brief testing for both controlled/uncontrolled cases, though I think it's worth adding tests. Marco and I were talking about how we might want to start adding a .each()
to our component tests so everything is tested in both controlled/uncontrolled cases. @chad1008 Do you want to try that here? It might not fit the scope of this PR, and this PR has been running long, so feel free to defer if it's too high-effort to include here. As long as we are reasonably confident that we haven't regressed, this PR can land.
|
||
// Sync incoming value with radio.state. | ||
useUpdateEffect( () => { | ||
if ( value !== radio.state ) { | ||
radio.setState( value ); | ||
} | ||
// disable reason: Adding `radio` to the dependency array causes an extra render. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we did something like
const setRadioState = radio.setState;
// ...
useUpdateEffect( () => {
if ( value !== radio.state ) {
setRadioState( value );
}
}, [ setRadioState, value ] );
That way, the hook will re-run only when setRadioState
changes (and not the whole radio
object). Furthermore, there's a chance that setState
is an actual React useState
setter, which means its value will be stable throughout the component's lifetime (although we should check for that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea - playing with it got interesting though:
If we go this route, we then need radio.state
as a dependency. ESLint wasn't asking for that before, perhaps because it would have been happy with the full radio
object?
Anyway - if we add that dependency, we end up (once again) with an additional render whenever a new option is selected, just like we'd get if we added radio
directly. I suppose that makes sense, since radio.state
is going to change every time.
This leaves me uncertain of the best course forward. Leave the disabling comment for now? Just add radio
as the linter originally wanted, rather than introduce a new variable that doesn't actually solve our issue? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Adding those dependencies sounds correct to me, though — we could apply the technique that I mentioned above to both state
and setState
, and do something like this
Click to expand
diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx
index 8cf75e2266..a2fc579d2b 100644
--- a/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx
+++ b/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx
@@ -64,14 +64,14 @@ function UnforwardedToggleGroupControlAsRadioGroup(
}, [ radio.state, previousValue, onChange ] );
// Sync incoming value with radio.state.
+ const { state: radioState, setState: setRadioState } = radio;
useUpdateEffect( () => {
- if ( value !== radio.state ) {
- radio.setState( value );
+ if ( value !== radioState ) {
+ setRadioState( value );
}
- // disable reason: Adding `radio` to the dependency array causes an extra render.
- // Memoizing it doesn't look easily doable, so we're disabling the rule for now.
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [ value ] );
+ // setRadioState needs to be listed even if in theory it's supposed to be a
+ // stable reference — that's an ESLint limitation.
+ }, [ radioState, setRadioState, value ] );
return (
<ToggleGroupControlContext.Provider
We'd need of course to perform some testing and make sure that this doesn't introduce regressions — but, in theory, the hook is relatively simple and it makes sense to me that we re-run the hook when the radio state updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a similar thought 🙂 I'll get those changes pushed now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple additional suggestions. On top of that, I think that it would be best to update the as-radio-group.tsx
file with the same pattern (using useControlledState
for handling controlled/uncontrolled updates, creating the context value separately with useMemo
), even if it's not 100% related to original scope of the PR
ee82b6c
to
e184c9c
Compare
Investigating test failures... |
64ef11f
to
3acacad
Compare
}, [ value ] ); | ||
|
||
const [ selectedValue, setSelectedValue ] = useControlledValue( { | ||
defaultValue: previousValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this line (and probably we don't need previousValue
at all anymore)
setState: setSelectedValue as React.Dispatch< | ||
React.SetStateAction< string | number | undefined > | ||
>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of typecasting setSelectedValue
, we can try to type useControlledValue
a bit most narrowly using const
assertions
diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
index 0de5543bcc..a9e7879071 100644
--- a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
+++ b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
@@ -60,9 +60,7 @@ function UnforwardedToggleGroupControlAsButtonGroup(
() => ( {
baseId,
state: selectedValue,
- setState: setSelectedValue as React.Dispatch<
- React.SetStateAction< string | number | undefined >
- >,
+ setState: setSelectedValue,
isBlock: ! isAdaptiveWidth,
isDeselectable: true,
size,
diff --git a/packages/components/src/utils/hooks/use-controlled-value.ts b/packages/components/src/utils/hooks/use-controlled-value.ts
index 5a3824d465..bb7f3aea80 100644
--- a/packages/components/src/utils/hooks/use-controlled-value.ts
+++ b/packages/components/src/utils/hooks/use-controlled-value.ts
@@ -22,7 +22,7 @@ export function useControlledValue< T >( {
defaultValue,
onChange,
value: valueProp,
-}: Props< T > ): [ T | undefined, ( value: T ) => void ] {
+}: Props< T > ) {
const hasValue = typeof valueProp !== 'undefined';
const initialValue = hasValue ? valueProp : defaultValue;
const [ state, setState ] = useState( initialValue );
@@ -40,5 +40,5 @@ export function useControlledValue< T >( {
setValue = setState;
}
- return [ value, setValue ];
+ return [ value, setValue as typeof setState ] as const;
}
Basically:
- in the return statement, we typecast
setValue
to be "whatever is the type of thesetState
function - we add
as const
, which basically tells TypeScript to interpret the type of the return statement as literally as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can actually open a separate PR with this change, so that this PR stays focused on ToggleGroupControl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#46164 (merged), we can rebase on top on trunk
to include those changes, which should allow to remove the as React.Dispatch< ...
type cast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making that change. I'm updating the typecasting now.
…d variables in the useUpdateEffects
Something odd I'm finding but haven't been able to sort out yet - with any of these changes, the |
3acacad
to
f4a0eca
Compare
So! I spent some time investigating this issue — here are my findings. InvestigationThe failing test has a peculiarity: it seems to test the component in a mixed controlled/uncontrolled way:
By applying this same scenario to the Storybook example, I was able to reproduce the same failure in Storybook — basically, since the Here are the changes that I made in Storybook that allow replicating the bugdiff --git a/packages/components/src/toggle-group-control/stories/index.tsx b/packages/components/src/toggle-group-control/stories/index.tsx
index 9978481764..6c21a9548f 100644
--- a/packages/components/src/toggle-group-control/stories/index.tsx
+++ b/packages/components/src/toggle-group-control/stories/index.tsx
@@ -6,7 +6,7 @@ import type { ComponentMeta, ComponentStory } from '@storybook/react';
/**
* WordPress dependencies
*/
-import { useState } from '@wordpress/element';
+// import { useState } from '@wordpress/element';
import { formatLowercase, formatUppercase } from '@wordpress/icons';
/**
@@ -20,7 +20,7 @@ import {
import type {
ToggleGroupControlOptionProps,
ToggleGroupControlOptionIconProps,
- ToggleGroupControlProps,
+ // ToggleGroupControlProps,
} from '../types';
const meta: ComponentMeta< typeof ToggleGroupControl > = {
@@ -43,17 +43,17 @@ const Template: ComponentStory< typeof ToggleGroupControl > = ( {
onChange,
...props
} ) => {
- const [ value, setValue ] =
- useState< ToggleGroupControlProps[ 'value' ] >();
+ // const [ value, setValue ] =
+ // useState< ToggleGroupControlProps[ 'value' ] >();
return (
<ToggleGroupControl
{ ...props }
onChange={ ( ...changeArgs ) => {
- setValue( ...changeArgs );
+ // setValue( ...changeArgs );
onChange?.( ...changeArgs );
} }
- value={ value }
+ // value={ value }
/>
);
};
@@ -133,4 +133,5 @@ export const Deselectable: ComponentStory< typeof ToggleGroupControl > =
Deselectable.args = {
...WithIcons.args,
isDeselectable: true,
+ value: 'uppercase',
}; I was then able to make the tests pass by playing around with the See first attempted fixdiff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
index a9e7879071..9b4191f750 100644
--- a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
+++ b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
@@ -9,7 +9,6 @@ import type { ForwardedRef } from 'react';
import {
useMergeRefs,
useInstanceId,
- usePrevious,
useResizeObserver,
} from '@wordpress/compose';
import { forwardRef, useRef, useMemo } from '@wordpress/element';
@@ -49,11 +48,9 @@ function UnforwardedToggleGroupControlAsButtonGroup(
ToggleGroupControlAsButtonGroup,
'toggle-group-control-as-button-group'
).toString();
- const previousValue = usePrevious( value );
const [ selectedValue, setSelectedValue ] = useControlledValue( {
- defaultValue: previousValue,
+ defaultValue: value,
onChange,
- value,
} );
// Expose selectedValue getter/setter via context
const groupContext: ToggleGroupControlContextProps = useMemo( Although that felt more like a hack that avoids the issue, rather than a fix. ConclusionsIt looks like Initial value vs current valueOn A better approach in my opinion would be for consumers of Diff showing how `defaultValue` prop could be used in `ToggleGroupControlAsButtonGroup`diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
index a9e7879071..8780e1a333 100644
--- a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
+++ b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
@@ -9,7 +9,6 @@ import type { ForwardedRef } from 'react';
import {
useMergeRefs,
useInstanceId,
- usePrevious,
useResizeObserver,
} from '@wordpress/compose';
import { forwardRef, useRef, useMemo } from '@wordpress/element';
@@ -34,6 +33,7 @@ function UnforwardedToggleGroupControlAsButtonGroup(
label,
onChange,
size,
+ defaultValue,
value,
...otherProps
}: WordPressComponentProps<
@@ -49,9 +49,8 @@ function UnforwardedToggleGroupControlAsButtonGroup(
ToggleGroupControlAsButtonGroup,
'toggle-group-control-as-button-group'
).toString();
- const previousValue = usePrevious( value );
const [ selectedValue, setSelectedValue ] = useControlledValue( {
- defaultValue: previousValue,
+ defaultValue: defaultValue as string | number,
onChange,
value,
} );
diff --git a/packages/components/src/toggle-group-control/toggle-group-control/component.tsx b/packages/components/src/toggle-group-control/toggle-group-control/component.tsx
index ebd4893e37..7a35c81a72 100644
--- a/packages/components/src/toggle-group-control/toggle-group-control/component.tsx
+++ b/packages/components/src/toggle-group-control/toggle-group-control/component.tsx
@@ -42,6 +42,7 @@ function UnconnectedToggleGroupControl(
onChange = noop,
size = 'default',
value,
+ defaultValue,
children,
...otherProps
} = useContextSystem( props, 'ToggleGroupControl' );
@@ -81,6 +82,7 @@ function UnconnectedToggleGroupControl(
ref={ forwardedRef }
size={ size }
value={ value }
+ defaultValue={ defaultValue }
/>
</BaseControl>
); Using
|
Thank you so much for digging into that @ciampo! I'll await @mirka's thoughts on your full conclusions, but my initial thought thought is that I also think it makes sense to pass an empty string instead of undefined (side note, is it wrong that I'm this excited that we had the same thought there? 😆 In addition to that, a quick question for my own edification - you've presented two options on the unit test (fully controlled vs. fully uncontrolled). Would it make sense to test both, or is that overkill? |
Yup, let's wait for Lena's thoughts!
I would personally like to test both controlled and uncontrolled, as testing for both behaviours can help us flag bugs and inconsistencies (like in this PR). |
My thoughts are basically this — the most important point being that I'm reluctant to adopt the "pass empty string in lieu of I agree that our tests should ideally cover both controlled/uncontrolled. In this particular PR though, I'm also fine with prioritizing landing just the eslintrc update with minimal changes, and defer any large component/test changes to a separate issue so it can be evaluated/prioritized separately. |
My gut tells me that we should respect the convention around passing But that raises the question about how to represent correctly an "empty but controlled" value for a given prop: passing an empty string may cut it for props with As discussed, probably the best next move is to open a separate issue where we discuss "controlled" vs "uncontrolled" strategy for the package. |
I agree with Marco in that a broader conversation sounds like it's in order before we move forward on this PR - I'll leave it as is for now, and revisit when we have a clearer picture of how we want to proceed with the controlled/uncontrolled distinction at the package level. |
Opened #47473 to discuss separately |
Edit — I got ahead of myself, it looks like there's more to that conversation. Let's probably wait a little bit more |
@chad1008 is this PR still relevant? |
What?
As discussed in #45044, now that
exhaustive-deps
is active for the Components package, it makes sense to includeuseUpdateEffect
in the check. There may be other custom hooks we want to include here, but they can be addressed in separate PRs as needed.Why?
For the same reason as the overall
exhaustive-deps
migration, keeping hook dependencies clean can help avoid unexpected behavior creeping into components.How?
Fortunately, there only appears to be one component triggering warnings related to
useUpdateEffect
, and it'sToggleGroupControl
. There are several warnings in a couple of different spots, more details to follow as this PR develops.With just one component in play, I don't think we need a separate followup PR, so I'll manage all of the work here.
Mostly straightforward changes here. Most notable:
previousValue
is actually a ref value provided by theusePrevious
hook - so changes won't cause re-renders. Normally it wouldn't be a dependency, buteslint
can't tell that it's actually a ref.onChange
to the dep array does cause the effect to fire more often when the function is defined/destructured, but it is not triggering any additional re-renders.groupContext
is defined on each render and therefor wouldn't be referentially stable, but wrapping inuseMemo
should resolve that.radio
to theas-radio-group
dep array triggers an additional render as the object gets re-declared each time. I didn't see an easily viable solution, so I've ignored this error for now. It's possible there is an easy fix that I've just overlooked due to the TypeScript of it all.Testing Instructions
npx eslint packages/components/src/
exhaustive-deps
rule, particularly related touseUpdateEffect